Add maxflow #24

TonalidadeHidrica · 2020-09-08T15:20:12Z

No description provided.

TonalidadeHidrica · 2020-09-08T18:09:36Z

I will add tests and an example code that is submittable to D - Maxflow, but I will open the PR now. Waiting everyone for review!

MiSawa · 2020-09-09T05:05:56Z

src/maxflow.rs

+            if res == up {
+                break;
+            }
+        }


Suggested change

}

}

self.iter[v] = self.graph.g[v].len();

This is an important bit of the algorithm that we should not omit. Alternatively we can do

self.level[v] = self.graph.g.len();

You're right. It is difficult to translate C++ for-statements with side effects...

MiSawa · 2020-09-09T05:07:05Z

src/maxflow.rs

+    pub fn add_edge(&mut self, from: usize, to: usize, cap: Cap) -> usize {
+        assert!(from < self._n);
+        assert!(to < self._n);
+        assert!(Cap::zero() < cap);


Suggested change

assert!(Cap::zero() < cap);

assert!(Cap::zero() <= cap);

MiSawa · 2020-09-09T05:08:12Z

src/maxflow.rs

+    pub fn add_edge(&mut self, from: usize, to: usize, cap: Cap) -> usize {
+        assert!(from < self._n);
+        assert!(to < self._n);
+        assert!(Cap::zero() < cap);


Suggested change

assert!(Cap::zero() < cap);

assert!(Cap::zero() < cap);

assert_ne!(from, to);

or fix rev calculation please.

Could you teach me why do we need this? The original documentation says that we can input self-loops to graphs, and I don't think that the code does not work for the situation. What change do I have to apply to rev calculation?

The original implementation has the same bug-ish behavior although both this and the original impl technically "work" as these edges won't be touched... I believe. Though it is better to fix, since this bug brakes a fundamental invariant of this graph structure.

The problem is that; when from == to, rev calculated in L:68 will be the index of the edge added in L:69, i.e. itself instead of the reverse edge.
Since rev should refer the index of the reversal auxiliary edge, it has to be calculated by

let rev = self.g[to].len() + if from == to { 1 } else { 0 };

Ah, I've understood. Fixing.

MiSawa · 2020-09-09T05:09:01Z

src/maxflow.rs

+    fn change_edge(&mut self, i: usize, new_cap: Cap, new_flow: Cap) {
+        let m = self.pos.len();
+        assert!(i < m);
+        assert!(Cap::zero() < new_flow && new_flow <= new_cap);


Suggested change

assert!(Cap::zero() < new_flow && new_flow <= new_cap);

assert!(Cap::zero() <= new_flow && new_flow <= new_cap);

MiSawa · 2020-09-09T05:16:31Z

src/maxflow.rs

+    pub fn flow(&mut self, s: usize, t: usize) -> Cap {
+        self.flow_with_capacity(s, t, Cap::max_value())
+    }
+    pub fn flow_with_capacity(&mut self, s: usize, t: usize, flow_limit: Cap) -> Cap {


What is the reason of this naming? It make sense to name this a capacity in some specific context, but here probably doesn't. Is there any reference that this is called capacity?
Maybe flow_with_limit, flow_with_value_limit or something like that?

I have an image of adding new edge from virtual vertex s' to vertex s with capacity flow_limit. But I agree that flow_with_limit may be more intuitive. I want to ask others' opinion for this point.

MiSawa · 2020-09-09T05:16:55Z

src/maxflow.rs

+    pub fn flow_with_capacity(&mut self, s: usize, t: usize, flow_limit: Cap) -> Cap {
+        let n_ = self._n;
+        assert!(s < n_);
+        assert!(t < n_);


Suggested change

assert!(t < n_);

assert!(t < n_);

assert_ne!(s, t);

I think I should not add this assertion, because the original documentation does not say it disallows the same vertices, and by definition we can return a valid value flow_limit.

By definition in the document appendix.html, we should return 0 I believe, which is very confusing as you might have noticed.

g(v, f) = Sum{...} - Sum{...}

This implies the sum of g(v, f) over all vertices is zero.

g(v, f) = g(v, f') holds for all vertices v other than s and t.

Since s = t, this also implies g(t, f) = g(t, f'). (∵ everywhere except t is fixed, and the sum is fixed)

g(t,f′)−g(t,f) is maximized under these conditions. It returns this g(t,f′)−g(t,f)

Since g(t, f) = g(t, f'), the returned value should be 0.

I finally understood. It seems to be true that it should return 0. Doesn't the current code?

I tried the original library with a simple example and it returned MAX_VALUE instead...

It doesn't return 0 if the limit wasn't 0, since s is reachable from itself, and v == self.s holds in the first call of dfs.

#[cfg(test)] mod test { use super::*; #[test] fn test() { let mut g = MfGraph::new(3); assert_eq!(0, g.flow_with_capacity(0, 0, 10)); } }

fails with

Left: 0 Right: 10

If you formulate the max flow problem in LP, the primal would be UNBOUND and the dual would be INFEASIBLE. So it also makes sense to return the infinity.

Since there are multiple way of interpreting the meaning of max flow for s == t case, I'd just add assert_ne.

Reported in atcoder/ac-library#5

ACL fixed it :)

MiSawa · 2020-09-09T05:20:32Z

src/maxflow.rs

+    fn max_value() -> Self;
+}
+
+impl MfCapacity for i32 {


Maybe use macro for these impls?

This is a preliminary implementation. The internal_type_traits implementation (#19) provides with more wide variety of features, so I will replace with it once it is merged.

MiSawa · 2020-09-09T05:21:31Z

src/internal_queue.rs

+        self.pos = 0;
+    }
+
+    pub(crate) fn pop(&mut self) {


Maybe fn pop(&mut self) -> Option<&T>?

That is an enhancement that is not in the original library, but it seems useful and safe, so I'll add it. The discussion about internal_queue is underway in #13, by the way.

@MiSawa

Thanks @MiSawa for your invaluable comments!

MiSawa · 2020-09-09T07:49:13Z

src/maxflow.rs

+    pub fn flow_with_capacity(&mut self, s: usize, t: usize, flow_limit: Cap) -> Cap {
+        let n_ = self._n;
+        assert!(s < n_);
+        assert!(t < n_);


It doesn't return 0 if the limit wasn't 0, since s is reachable from itself, and v == self.s holds in the first call of dfs.

#[cfg(test)] mod test { use super::*; #[test] fn test() { let mut g = MfGraph::new(3); assert_eq!(0, g.flow_with_capacity(0, 0, 10)); } }

fails with

Left: 0 Right: 10

If you formulate the max flow problem in LP, the primal would be UNBOUND and the dual would be INFEASIBLE. So it also makes sense to return the infinity.

Since there are multiple way of interpreting the meaning of max flow for s == t case, I'd just add assert_ne.

MiSawa · 2020-09-09T07:50:57Z

src/maxflow.rs

+    pub fn flow_with_capacity(&mut self, s: usize, t: usize, flow_limit: Cap) -> Cap {
+        let n_ = self._n;
+        assert!(s < n_);
+        assert!(t < n_);


Plus

Suggested change

assert!(t < n_);

assert!(t < n_);

assert!(Cap::zero() <= flow_limit);

maybe?

MiSawa · 2020-09-10T02:50:13Z

src/maxflow.rs

+    pub fn flow_with_capacity(&mut self, s: usize, t: usize, flow_limit: Cap) -> Cap {
+        let n_ = self._n;
+        assert!(s < n_);
+        assert!(t < n_);


Reported in atcoder/ac-library#5

TonalidadeHidrica · 2020-09-11T07:06:48Z

@MiSawa Sorry to be late. I think I added all the assertions you mentioned and fixed all the mistakes. Now I'm going to add some tests. Do you have any good idea of test graphs?

MiSawa · 2020-09-11T08:53:16Z

@MiSawa Sorry to be late. I think I added all the assertions you mentioned and fixed all the mistakes. Now I'm going to add some tests. Do you have any good idea of test graphs?

Thank you for being patient about all my nitpicks!

This example in Wikipedia has unique optimal solution and very simple. Maybe good as the first test.
Here's unit tests for the original library.
Here's a test case for a common mistake of Dinic algorithm's implementations, although I know the current version is fine.

MiSawa

LGTM except for missing tests :)

MiSawa · 2020-09-11T09:02:23Z

src/maxflow.rs


+impl<Cap> MfGraph<Cap>
+where
+    Cap: Integral,


Actually the algorithm doesn't rely on the integrality.... Though it does rely on Ord. Not sure what we should do.

One possible way is to define a trait dedicated for max-flow capacities, which is my previous implementation. Refer to this commit to see the differences. However this is a bit like a repetition of Integral trait in internal_type_traits module, that's why I simply reused it.
At least the original library guarantees only int and ll, I guess this is already sufficient.

By the way, can this implementation be applied to floating-point capacity? I mean, don't we need some additional treatment of precision errors?

Good question!
I believe so long as min(x, y) in {x, y} and x-x = 0, the algorithm work fine-ish. e.g. for floating point numbers without NaN / inf, polynomial, etc.
I said fine-ish since although the time complexity doesn't get worse, there are errors that are unavoidable without using multi-precision floating point number. Guaranteeing the accuracy of the returned flow value and flows on edges would be hard. Farthemore since cut is discrete, the min-cut verticis can be different from the correct one although the cut value should be similar.

In competitive programming, I think I have seen some problems that uses polynomial flows (or big-M instead) and floating point numbers (came from binary/ternary search, can be transformed to a search on rational numbers though require a lot more coding).
Though maybe it will be very rare in AtCoder at least, since ACL doesn't support them.

TonalidadeHidrica · 2020-09-11T09:19:03Z

About the tests:

Graphs from Wikipedia is licensed under CC-BY-SA 3.0, so we may use them as long as we mention the reference.
The license for test cases of original libraries is still yet to be determined, so I'll ask the author if I can use it.
@MiSawa Under which license can I use your test case?

(I'm not a lawyer, I'm not confident at all.. After all, do I even have to consider the licenses about those small graphs?)

MiSawa · 2020-09-11T09:32:13Z

@TonalidadeHidrica

* Graphs from Wikipedia is licensed under CC-BY-SA 3.0, so we may use them as long as we mention the reference.

I think CC-BY-SA 3.0 applies to the picture itself, not to the graph instance that is represented by the picture. Though not really sure, and it's safer to mention the origin anyway.

* @MiSawa Under which license can I use your test case?

NYSL! Added a comment in the gist :)

TonalidadeHidrica · 2020-09-11T12:20:59Z

~~Why does clippy emit warnings in some platforms but not all??~~ It's not because of platform but the version.

MiSawa · 2020-09-12T09:44:54Z

src/maxflow.rs

+
+    #[test]
+    #[allow(clippy::many_single_char_names)]
+    fn test_max_flow_misawa() {


You might have done already, but verified locally that it gonna take very long with n=20 and take forever with n=100 when we deleted L:189 and L:209 as intended :)

TonalidadeHidrica · 2020-09-13T04:22:55Z

@qryxip Maybe we can merge this already.

TonalidadeHidrica marked this pull request as draft September 8, 2020 15:20

Add maxflow

1ff54bd

TonalidadeHidrica changed the title ~~Adding maxflow~~ Add maxflow Sep 8, 2020

TonalidadeHidrica marked this pull request as ready for review September 8, 2020 18:07

Remove unneeded return

d4b6324

MiSawa reviewed Sep 9, 2020

View reviewed changes

TonalidadeHidrica added 6 commits September 9, 2020 14:39

Merge branch 'feature/internal_queue' into feature/maxflow

91ac693

Fixed some mistakes

67a48d0

Thanks @MiSawa for your invaluable comments!

Handle self-loop in add_edge

ae780c9

Change visibility of some functions and structs

693cfdb

Added sample code for D - Maxflow

55c0d81

Merge branch 'feature/internal_queue' into feature/maxflow

a9bb6e8

MiSawa reviewed Sep 10, 2020

View reviewed changes

TonalidadeHidrica added 6 commits September 11, 2020 15:32

Merge branch 'master' into feature/maxflow

43dfa84

Merge branch 'feature/internal_queue' into feature/maxflow

b9ed34c

Follow the changes of interface in internal_queue

abbd181

Suppress warnings

a793068

Added additional assertions and documentation

51a9c98

Replace MfCapacity with Integral from type traits

c05ac18

MiSawa approved these changes Sep 11, 2020

View reviewed changes

Add tests

7365855

TonalidadeHidrica added 2 commits September 11, 2020 21:24

Suppress too many single-character variables warnings

125fc64

Merge branch 'master' into feature/maxflow

1c8d2ab

MiSawa reviewed Sep 12, 2020

View reviewed changes

MiSawa approved these changes Sep 12, 2020

View reviewed changes

qryxip merged commit 5103743 into rust-lang-ja:master Sep 13, 2020

TonalidadeHidrica deleted the feature/maxflow branch September 13, 2020 04:27

	assert!(Cap::zero() < cap);
	assert!(Cap::zero() < cap);
	assert_ne!(from, to);

	assert!(Cap::zero() < new_flow && new_flow <= new_cap);
	assert!(Cap::zero() <= new_flow && new_flow <= new_cap);

	assert!(t < n_);
	assert!(t < n_);
	assert!(Cap::zero() <= flow_limit);

Add maxflow #24

Add maxflow #24

Conversation

TonalidadeHidrica commented Sep 8, 2020

TonalidadeHidrica commented Sep 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MiSawa Sep 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MiSawa Sep 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TonalidadeHidrica commented Sep 11, 2020

MiSawa commented Sep 11, 2020

MiSawa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TonalidadeHidrica commented Sep 11, 2020

MiSawa commented Sep 11, 2020 • edited Loading

TonalidadeHidrica commented Sep 11, 2020 • edited Loading

MiSawa Sep 12, 2020 • edited Loading

Choose a reason for hiding this comment

TonalidadeHidrica commented Sep 13, 2020

MiSawa Sep 9, 2020 •

edited

Loading

MiSawa Sep 9, 2020 •

edited

Loading

MiSawa commented Sep 11, 2020 •

edited

Loading

TonalidadeHidrica commented Sep 11, 2020 •

edited

Loading

MiSawa Sep 12, 2020 •

edited

Loading